Emit error when using path-segment keyword as cfg pred #146978

mu001999 · 2025-09-24T13:21:27Z

Fixes #146968

Emit error CfgPredicateIdentifier if the word is path-segment keyword.

Detailed change description - #146978 (comment).

r? petrochenkov

rustbot · 2025-09-24T13:21:30Z

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

petrochenkov · 2025-09-26T15:44:50Z

Could you

add similar test cases for some non path-segment keyword, like struct
add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg].

After that we should be able to run crater on this.
@rustbot author

rustbot · 2025-09-26T15:44:55Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

mu001999 · 2025-09-27T03:26:50Z

add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg]

--cfg will emit same error but will be suppressed because parse_cfg is called with ParseSess::with_fatal_emitter

mu001999 · 2025-09-27T03:27:17Z

@rustbot ready

petrochenkov · 2025-09-30T15:01:21Z

add test cases for --cfg on command line and make sure that they behave identically to cfg! and #[cfg]

--cfg will emit same error but will be suppressed because parse_cfg is called with ParseSess::with_fatal_emitter

with_fatal_emitter doesn't suppress errors, it makes them fatal.
--cfg parsing has its own error reporting logic in fn parse_cfg in compiler\rustc_interface\src\interface.rs, and the new errors are not emitted there.
@rustbot author

mu001999 · 2025-09-30T15:34:26Z

with_fatal_emitter doesn't suppress errors, it makes them fatal.

@petrochenkov I found with_fatal_emitter create dcx with FatalOnlyEmitter, see

rust/compiler/rustc_session/src/parse.rs

Lines 318 to 329 in a2db928

    
           pub fn with_fatal_emitter(locale_resources: Vec<&'static str>, fatal_note: String) -> Self { 
        
               let translator = Translator::with_fallback_bundle(locale_resources, false); 
        
               let sm = Arc::new(SourceMap::new(FilePathMapping::empty())); 
        
               let fatal_emitter = 
        
                   Box::new(HumanEmitter::new(stderr_destination(ColorConfig::Auto), translator)); 
        
               let dcx = DiagCtxt::new(Box::new(FatalOnlyEmitter { 
        
                   fatal_emitter, 
        
                   fatal_note: Some(fatal_note), 
        
               })) 
        
               .disable_warnings(); 
        
               ParseSess::with_dcx(dcx, sm) 
        
           }

and the comment of FatalOnlyEmitter and the impl of FatalOnlyEmitter::emit_diagnostic show only errors with diag.level == Level::Fatal can be emitted, see

rust/compiler/rustc_errors/src/emitter.rs

Lines 537 to 562 in 8155734

    
           /// An emitter that does nothing when emitting a non-fatal diagnostic. 
        
           /// Fatal diagnostics are forwarded to `fatal_emitter` to avoid silent 
        
           /// failures of rustc, as witnessed e.g. in issue #89358. 
        
           pub struct FatalOnlyEmitter { 
        
               pub fatal_emitter: Box<dyn Emitter + DynSend>, 
        
               pub fatal_note: Option<String>, 
        
           } 
        
           impl Emitter for FatalOnlyEmitter { 
        
               fn source_map(&self) -> Option<&SourceMap> { 
        
                   None 
        
               } 
        
               fn emit_diagnostic(&mut self, mut diag: DiagInner, registry: &Registry) { 
        
                   if diag.level == Level::Fatal { 
        
                       if let Some(fatal_note) = &self.fatal_note { 
        
                           diag.sub(Level::Note, fatal_note.clone(), MultiSpan::new()); 
        
                       } 
        
                       self.fatal_emitter.emit_diagnostic(diag, registry); 
        
                   } 
        
               } 
        
               fn translator(&self) -> &Translator { 
        
                   self.fatal_emitter.translator() 
        
               } 
        
           }

mu001999 · 2025-09-30T15:36:18Z

I have debug the logic in fn parse_cfg, and the err.emit is called in fact, but it is suppressed by the FatalOnlyEmitter.

petrochenkov · 2025-09-30T15:52:17Z

Ah, ok, "fatal emitter" means "fatal-only emitter".

In any case, the behavior is not correct.
I guess the assumption when using the fatal emitter was that we always reach the "expected key or key="value"" error at the bottom anyway, but that's not actually the case.
If we get to return (ident.name, meta_item.value_str()), then all the non-fatal errors are lost, which is incorrect.

petrochenkov · 2025-10-23T11:47:02Z

Nice, there are only spurious regressions.
I'll write the change description for the lang team later today and pass it to them.

mu001999 · 2025-10-23T12:14:42Z

@petrochenkov Cool! Thanks!

petrochenkov · 2025-10-23T14:31:50Z

Change introducion

This PR makes syntax accepted by cfg predicates in #[cfg] attributes and cfg! macros stricter.
It also makes predicates in --cfg command line options accept the same syntax as in the attributes and macros (except true and false).

`#[cfg]` and `cfg!`

Predicates are generally supposed to use non-keyword identifiers (cfg!(ident), cfg!(ident="value")) + boolean literals, and attributes and macros generally followed this grammar, with one exception.
The exceptions is that path segment keywords (super, self, Self, crate, expanded $crate) were accepted.

This happened because internally cfg predicates were parsed as meta-items (besides boolean literals), and then inappropriate meta-items were rejected after obtaining the parsing result.
Path segment keywords are syntactically valid paths, and therefore valid meta-items, so they were successfully parsed but accidentally not rejected by a post-processing check.

This PR rejects them.

`--cfg`

Predicate syntax in --cfg was parsed largely incorrectly due to majority of parsing errors being suppressed.
Internally --cfg parsing used so called FatalOnlyEmitter overriding the regular error emitter, and that emitter just ignored all recoverable errors.
Initially a different emitter was introduced in "Hide diagnostics emitted during --cfg parsing" to improve diagnostics (reduce noisiness), but the actual effect was also occasionally accepting invalid syntax. "Report fatal lexer errors in --cfg command line arguments" fixed up the situation a bit, but not completely.
Eventually parser improved and improved its recovery, produced more and more recoverable errors, and FatalOnlyEmitter became more and more wrong, leading to the current situation.
In particular, the --cfg true/--cfg false case considered by lang team in #138632 (comment) was an instance of errors swallowed by FatalOnlyEmitter, it is not syntactically valid (true and false are not even meta-items).

This PR disables the error suppression in --cfg parsing and makes it accept the same syntax as its counterparts in #[cfg] and cfg! (except true and false because those are not predicate definitions).

Breakage

Crater run above (#146978 (comment)) didn't find any regressions from this.

traviscross · 2025-10-29T17:31:45Z

Sounds right, thanks @petrochenkov.

@rfcbot fcp merge

rust-rfcbot · 2025-10-29T17:31:47Z

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

nikomatsakis · 2025-10-29T17:31:48Z

@rfcbot reviewed

tmandry · 2025-10-29T17:32:07Z

@rfcbot reviewed

rust-rfcbot · 2025-10-29T17:32:15Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 24, 2025

rustbot assigned petrochenkov Sep 24, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2025

mu001999 force-pushed the fix/path-kw-as-cfg-pred branch from 505d13f to b2be57d Compare September 27, 2025 03:24